feat(web): report the runtime inputs a connector declares - #2132
Conversation
Two owner-scoped read endpoints report which runtime inputs an agent's connectors declare and which of them a task already has: one keyed by agent for a pre-flight check, one keyed by task for the in-chat prompt. Both return declared key names, their normalized type and whether a value is already stored - never a stored value, and never a connector's URL, headers, environment or authentication configuration. Both live on the chat router, next to the task-keyed runtime-extensions endpoint they are shaped after, and both reuse the predicate task creation already applies to an agent id rather than introducing a second authorization path for the same resource. The team scope both endpoints resolve is pinned to the one tool loading uses at run time, so a team-shared connector that the run-time gate demands a value for is always one the caller can also see here. Neither endpoint asserts that required values are present: reporting what is missing is the whole point, and raising on a missing value belongs to the per-turn gate that runs later.
Task creation now returns the same requirements report as the read endpoints, so a client can prompt for the missing values before sending the first message instead of after a failed turn. The request body is unchanged and still ignores any runtime values a caller smuggles in. The report reuses the connector resolution the creation path already performs, so it costs no extra query. The field is null rather than a report on the public widget and share-link create paths: those callers are anonymous guests who never see a connector's declared key names, so evaluating requirements for them would hand that information to a new audience this change does not intend to reach. The field still always appears in the response body either way.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces endpoints and services to manage connector runtime requirements and values, including GET endpoints for agent and task requirements, and a POST endpoint to merge context values with a compare-and-swap mechanism. It also adds comprehensive unit and integration tests, including PostgreSQL-specific concurrency tests. The review feedback suggests optimizing dictionary and object comparisons in the service layer by replacing redundant JSON serialization-based comparisons with Python's native equality operators, which improves performance.
4bcb602 to
10d04f4
Compare
AlexLiu190625
changed the title
|
The value-submission endpoint (POST /api/chat/task/{task_id}/connector-runtime-values) |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
This PR adds two read-only GET endpoints (agent-keyed and task-keyed) that report which runtime inputs a connector declares (e.g. auth_token) and whether each already has a stored value, plus a connector_runtime_requirements field on the task-create response. It's explicitly scoped as discovery/reporting only for #1251 -- the actual fix (UI dialog + value-submission endpoint) ships in a follow-up PR. The read-only layer is a reasonable prerequisite for that dialog, and the negative-path/auth handling is careful, but the PR's core positive-case output (satisfied: true) has zero test coverage anywhere in the suite.
Blocking: yes -- recommended event: REQUEST_CHANGES -- the boolean this PR exists to compute (satisfied: true for a stored, required runtime input) has no test asserting it, on either new endpoint or the task-create response.
Approach verdict: acceptable-with-reservations
- Top-level
satisfiedis permanently unachievable for any connector with a required secret in this phase -- it ANDs together sections that are structurally never satisfiable yet. That's a documented, intentional constant per the PR's own Disclosure 2, but there's no per-input discriminator letting a future consumer distinguish "not yet collected" from "not collectable this phase." Worth considering before the follow-up UI builds logic on top of this shape. - The task-keyed endpoint resolves the task's agent via a raw query instead of the established
_load_agent_for_task_runtimehelper that the real per-turn execution path uses (see finding #2 below). For workforce-manager agents or agents whose visibility has since changed, this can make the report list team-shared connectors the real per-turn execution would never resolve. Over-reporting only, narrow blast radius, but worth aligning report-vs-reality. - The two new endpoints disagree on admin-bypass scope (task-keyed denies admins, agent-keyed inherits an existing unconditional admin bypass) with no stated principle for the asymmetry. The bypass itself is pre-existing/inherited and already disclosed by the author, not introduced here -- but the inconsistency between sibling endpoints is worth a documented rationale or unification.
- The task-create response's
connector_runtime_requirementsfield is functionally redundant with what the agent-keyed GET already reports for the same agent/user, and there's currently no write path that could act on it post-create (context is immutable after creation). Not a defect in this PR, but worth flagging as design input for whoever builds the follow-up's value-write endpoint. - The PR's framing ("part of fixing #1251") is honest about scope, but worth a one-line reminder: #1251's actual failure mode (task created despite unmet requirement, later turns 400) is 100% unchanged by this PR alone -- this PR only adds visibility, not prevention.
Findings by severity
Major
1. [MAJOR, BLOCKING] src/xagent/web/services/connector_runtime.py:909-913, and tests/web/test_connector_runtime_entrypoints_e2e.py (entire file) -- No test anywhere asserts satisfied: true for any per-key input on the task-keyed endpoint or the task-create response. Every satisfied assertion in the suite checks False except one vacuous zero-connectors True case. The one real TaskConnectorRuntimeContext row ever created in tests is deliberately routed to the agent-keyed endpoint (which always reports False by contract) to test scope isolation -- never to the task-keyed endpoint where the stored-context branch actually matters. Hardcoding satisfied = False unconditionally in _build_connector_report would currently pass the entire suite. This is the exact value the planned follow-up UI dialog will gate re-prompting on; a silent regression here ships undetected and produces an incorrect user-visible result. Needs a test: task with a stored TaskConnectorRuntimeContext row for a required key -> task-keyed GET (and/or task-create response) reports that key satisfied: true, and top-level satisfied: true when it's the only required key.
Minor
2. src/xagent/web/api/chat.py:5363-5367 -- Task-keyed endpoint resolves the task's agent via a raw db.query(Agent).filter(Agent.id == task.agent_id).first() instead of _load_agent_for_task_runtime (services/llm_utils.py:1250-1306), which the real per-turn execution path uses and which returns None for a workforce-generated manager agent or since-revoked-visibility agent (falling back to personal-only connectors). The new endpoint always uses the raw agent's real team_id, so it can report team-shared connectors the actual turn would never resolve. Untested for this scenario on the task-keyed endpoint. Recommend routing through _load_agent_for_task_runtime for report/reality parity, plus a regression test.
6. src/xagent/web/services/connector_runtime.py:435 (docstring of the new resolve_agent_runtime_requirements, referencing prepare_connector_runtime_selection_snapshot) vs the new resolve_agent_runtime_requirements -- both independently derive the same connector-ref set via the same underlying calls, which the new function's own docstring warns against ("do not derive the refs any other way"). The old function is still live with 4 unchanged callers. A regression test catches drift after the fact but doesn't prevent it structurally. Recommend having the old function delegate to the new one. Separately, the new function's docstring claims refs are persisted "verbatim... in exactly that order," but both write (_sort_connector_refs) and read (_load_task_selected_refs) paths re-sort by (connector_type, connector_id) -- the actual invariant is canonical sorting, not order-preservation. Please correct the docstring.
7. src/xagent/web/services/connector_runtime.py:897 (_build_connector_report, via the pre-existing _has_runtime_declaration) -- runtime_input_schema OR runtime_bindings means a connector with runtime_bindings but no runtime_input_schema produces a report entry with only name+ref and an empty inputs: [] -- inert noise, doesn't affect satisfied or leak anything new. Separately, no test anywhere exercises a custom_api-type connector on these new endpoints -- every test uses MCPServer fixtures only. Worth adding coverage.
8. src/xagent/web/services/connector_runtime.py:908 (_build_connector_report, vs. the pre-existing _require_context_values) -- validates declared-key syntax via validate_runtime_source_key and raises on a malformed key, but _build_connector_report runs no equivalent validation, so a connector with a malformed declared key is reported normally (possibly satisfied: true) while the per-turn execution gate would hard-fail. Practically reachable since runtime_input_schema has no key-format validation at connector create/update time (custom_api.py, mcp.py). Recommend applying the same validation (or at least flagging invalid keys) in the report path.
9. src/xagent/web/services/connector_runtime.py:441-445 (resolve_agent_runtime_requirements docstring) -- the satisfied field means different things depending on which endpoint returns it: agent-keyed answers "has no required input at all" (per this docstring), task-keyed answers "every required input has a stored value." Same field name, same schema, undocumented semantic difference, while the module docstring implies both endpoints return the same shape of report. Please document the distinction explicitly.
10. src/xagent/web/services/connector_runtime.py:902-906 -- no test exercises the auth_selector section at all (neither the MCP branch that emits it, nor the custom_api skip-branch here). Removing this continue wouldn't fail any existing test (no fixture stores an auth_selector key in a custom_api connector's schema). Not a security issue -- the report only shows declared key names from the connector owner's own schema, and _validate_values_against_schema independently rejects any real auth_selector value submitted for a non-MCP connector regardless of what the report shows -- but worth a follow-up test to catch drift.
11. tests/web/test_connector_runtime_entrypoints_e2e.py:1380 -- expired (always False in this phase, per the schema's own docstring -- a deliberate frozen-contract placeholder, not dead flexibility) is never asserted anywhere in this file despite being a wire field. Recommend adding an assertion for it here, mirroring the existing secrets_expires_at is None assertion pattern used at this and other call sites. Optionally consider pinning secrets_expires_at's wire format (ISO string vs epoch) now, since the contract is meant to be frozen.
12. tests/web/test_connector_runtime_entrypoints_e2e.py:1982 -- this is currently the only satisfied: true assertion in the file, and it's the vacuous zero-connectors case. Consider adding a real positive case: an agent-keyed GET where the connector's only runtime inputs are optional (not required), which should also report satisfied: true but currently has no test. Also untested elsewhere: a task with agent_id IS NULL on the task-keyed endpoint (degrades gracefully, just unverified), and a genuinely nonexistent task_id/agent_id vs. one that exists but isn't owned by the caller.
13. tests/web/test_connector_runtime_entrypoints_e2e.py:2254-2256 -- dangling section header comment (# A4: POST /task/{task_id}/connector-runtime-values.) with zero tests under it, for an endpoint confirmed (per the author's own PR comment) to have moved to the follow-up PR. Please drop until that PR lands. Separately, test_agent_requirements_endpoint_bypassing_team_resolver_hides_shared_connector (line 1677) is a near-duplicate of the first half of test_team_shared_connector_visible_across_read_endpoints (line 1525), and its own docstring admits it can't actually express the "bypass team resolver" mutation it's named after. Consider folding it in or renaming/clarifying it.
Note on prior review
Prior gemini-code-assist findings (2 inline comments about _canonical_json comparison redundancy in a POST value-merge endpoint) concerned code that has since moved to a follow-up PR -- confirmed no POST endpoint or merge logic exists anywhere in this PR's current diff. Not re-opened or re-litigated here.
Simplification opportunities
shrink: hoist the repeated 3-line comment +connector_runtime_requirements=None(src/xagent/web/api/public_chat_access.py:1039-1042, 1147-1150, 1220-1223, 1312-1315) into a single module-level constant_NO_CONNECTOR_RUNTIME_REQUIREMENTS = None, referenced at eachTaskCreateResponse(...)call site.
net: -12 lines possible.
Blocking status & recommended decision
- Blocking: yes
- Recommended event: REQUEST_CHANGES
- Blocking issues:
src/xagent/web/services/connector_runtime.py:909-913&tests/web/test_connector_runtime_entrypoints_e2e.py, major, thesatisfied=Trueboolean this PR exists to compute has zero positive-branch test coverage -- a regression here ships undetected, [new].
The task-keyed requirements endpoint resolved the task's agent with a raw query, so it derived the connector team scope from the raw row instead of the scope a turn actually runs under. It now resolves the agent with the same two calls the per-turn tool build makes for the task, in the same order, so the report can neither over- nor under-report team-shared connectors. The report also treated a declared key whose syntax the per-turn gate rejects as an ordinary key, so a stored value under that name could make the report read satisfied for a task no turn can run. Such a key is now always reported unsatisfied. The report still never raises, and the per-turn gate still fails the turn on the same key. Adds the missing coverage for the report's positive branch: a stored context value reported as satisfied, a task whose agent the runtime resolves to none, an unfillable declared key, a custom_api connector, the auth_selector skip, and the expired field. Drops a section header left behind for tests that belong to a later change. Hoists the four identical "no requirements on this path" comments in the public create paths into one named constant.
The response schema now says which question `satisfied` answers on each endpoint that returns it, that `section` is what separates "not supplied yet" from "not supplyable at this phase", and that `expired` is a constant in every section rather than only in the secret-bearing ones. The agent-keyed resolver's docstring no longer claims the persisted refs keep the caller's order: both the write and the read of that column sort canonically.
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Design
Approach verdict: acceptable-with-reservations — see the satisfied docstring/implementation mismatch in finding 2.
Major
src/xagent/web/services/connector_runtime.py:911-961(also:1046-1077) — A declared key withrequired: falseand a malformed name is skipped by_all_required_inputs_satisfied, so the report can saysatisfied: true, yet_require_context_values/_require_ephemeral_valuesvalidate every declared key's syntax and will 400 that same key at run time;custom_api.py/mcp.pyapply no name validation at connector create/update, so this is reachable and untested. Validate declared-key syntax at connector create/update time, or count non-required-but-malformed keys as unsatisfied in the report.
Minor
src/xagent/web/api/chat.py:4493+src/xagent/web/schemas/connector_runtime.py:80-88+src/xagent/web/schemas/chat.py:198-209— The create response computesconnector_runtime_requirementsvia the agent-keyed producer (stored_context=Nonealways), but its docstring claims the task-keyed meaning "every required input of this task already has a value". Compute it via the task-keyed producer post-persist, or correct the docstring to say it is the agent-keyed answer.src/xagent/web/services/connector_runtime.py:882-946(esp.:928) vs:1146-1168—secrets/auth_selectorare hardcodedsatisfied=Falseeven when a deployment installed aset_connector_runtime_resolverhook, so tasks execute fine via_resolve_runtime_valueswhile the report permanently claims those inputs are unsatisfied. Consult the resolver when present, or scope the unsatisfied claim to the no-resolver default.src/xagent/web/services/connector_runtime.py(auth_selector branch for MCP connectors, near the custom_apicontinueskip) — Only the custom_api skip path is tested; no test creates an MCP connector with a requiredauth_selectorkey and asserts it appears as a populated section entry. Add that positive-case test.tests/web/test_connector_runtime_entrypoints_e2e.py:2085— Ownership coverage is only the non-admin intruder case. Add: admin reading another user's task (the disclosed no-admin-exception behavior), a task withagent_id IS NULL, a nonexistenttask_id, and an agent-keyed GET whose declared inputs are all optional (non-vacuoussatisfied: true).src/xagent/web/schemas/connector_runtime.py:52-53—section: strandtype: strdocument exactly 3 and 2 allowed values in prose only. UseLiteral[...]so the enumeration reaches the OpenAPI schema.
Blocking: no — recommended event: APPROVE
…red key The top-level `satisfied` flag counted only required inputs, so a connector declaring an optional key whose name the per-turn gate rejects reported a task as ready to run while every turn on it answers 400: both `_require_context_values` and `_require_ephemeral_values` validate the syntax of every declared key and raise before they look at `required`. `_all_required_inputs_satisfied` now re-checks every listed key through the same validator, so a malformed key holds the flag at false whether or not it is required. The key stays listed, its own `satisfied` stays false, and the report still never raises. Type `section` and `type` as literals so the closed sets both fields already documented in prose reach the OpenAPI schema. Correct what the task-create response claims: it is computed from the agent before the task is persisted, so it answers what a task created from that agent needs, not what the new task still misses. Only the task-keyed read consults stored values. Tests: a malformed optional key holding the flag false; an MCP connector declaring an `auth_selector` key; a non-vacuous satisfied report whose declared inputs are all optional; a task with no agent; an unknown id on both read endpoints; and an admin denied another user's task.
|
Follow-ups from the approval are in af86727 (CI running):
Suite: 58 passed for the two touched test files. |
3 participants
What
Server-side support for reporting the runtime inputs a connector
declares: two read endpoints, and the same report on the task-create
response.
Why
Part of #1251. A connector can declare a required runtime
input (for example an auth token), and today nothing in the web chat path
ever asks the caller for that input's value: the task is created, the
first turn is queued, and only then does execution fail because the value
was never collected. The caller's only workaround is to remove the
connector.
Scope: no UI in this PR
The chat dialog that calls these endpoints ships in a following PR. This
PR is deliberately server-only, so that the concurrency contract on the
value endpoint and the browser-side interaction get separate review
passes. The value-submission endpoint itself ships in the PR that follows
this one, once this one merges.
The two endpoints below already have a concrete consumer: the dialog in
that follow-up PR, by way of the value-submission endpoint that precedes
it there. Their response shape, field types, and empty-value behavior are
fixed by this PR, so this is not a "build it now, wire it up later"
endpoint -- the follow-up PR's implementation is constrained by what
ships here, not the other way around.
Splitting the work this way is intentional: the value endpoint's
concurrency contract (per-key merge, conditional update, bounded retry)
and the browser-side interaction are two different things to review, and
mixing them into one PR would dilute both.
How
Two read endpoints report the runtime inputs a connector
declares and which of them already have a value:
GET /api/chat/agent/{agent_id}/connector-runtime-requirements(beforea task exists) and
GET /api/chat/task/{task_id}/connector-runtime-requirements(for atask that already exists). Neither returns a stored value itself, and
neither returns a connector's URL, headers, environment, or
authentication configuration.
The agent-keyed endpoint lives on the chat router, under
/api/chat/agent/{agent_id}/...rather than under/api/agents, nextto the existing task-keyed
/api/chat/task/{task_id}/runtime-extensionsendpoint it is shaped after. It reuses the same predicate task creation
already applies to an agent id, so it needs no second authorization path
for the same resource, and it does not require moving that predicate out
of the module it already lives in.
Task creation (
POST /api/chat/task/create) now returns the same reporton its response, reusing the connector resolution it already performs,
so this costs no extra query. The request body is unchanged. The new
response field,
connector_runtime_requirements, is always present onevery task-create response: it carries a real report on this path, and
is
nullon the public widget and share-link create paths, whererequirements are never evaluated for the caller -- those callers are
anonymous visitors who should not receive a connector's key names.
This field is not a duplicate of the agent-keyed endpoint's answer. It
saves the create path a second round trip, and because it is always
present the follow-up client needs no "field absent means an older
backend" branch. It answers the agent-keyed question, not the
task-keyed one: it is computed from the agent before the new task is
persisted, so no value can have been stored against that task yet, and
every input on it reads unsatisfied. Only the task-keyed read endpoint
consults stored values and answers whether a task can run now.
Neither read endpoint asserts that a required value is present --
reporting what is missing is the point, and raising on a missing value
belongs to the per-turn execution gate that already exists and is
unchanged by this PR.
The top-level
satisfiedisfalsewhile any reported connectordeclares a key whose name that per-turn gate rejects, whether or not
the key is required: the gate validates the syntax of every declared
key before it looks at
required, so such a task fails every turn. Thekey stays listed with its own
satisfiedfalse, and the report itselfstill never raises. Validating key syntax when a connector is created
or updated is the real fix and belongs in a separate change.
Disclosures
These two new read endpoints return a connector's runtime key names
(for example
auth_token,tenant_id) to their caller. They neverreturn a stored value, and never return a connector's URL, headers,
environment variables, command, or authentication configuration.
Four groups of caller can reach these key names: the agent's owner; a
teammate the agent is shared with at the team level; any admin,
unconditionally, through a query path with no additional filter; and
any user a deployment's own policy implementation chooses to allow --
the default policy implementation in this repository allows no one, so
this group is empty by default, and its width is defined by the
deployment, not by this repository.
There is also a more direct path to the same information: any
logged-in user who can create a task against an already-published agent
becomes that task's owner on creation, and the task-keyed read
endpoint's ownership check passes for them without going through the
agent-keyed endpoint at all. This reuses task creation's existing
authorization behavior; this PR neither widens nor narrows it.
Key names are not a protected field: a connector's owner can write
anything into a key name, including a word like
passwordorapi_key, and the declared schema only validates a key name's syntax-- the system has no notion of a public or confidential key name.
Accepting this audience is a deliberate tradeoff: a caller who does not
know which keys to fill cannot use the connector in chat at all, which
is the problem this change exists to fix.
A connector that declares a required
secretsinput can never bereported as satisfied at this phase. No secret store exists yet, so
every
secrets(and, for an MCP connector,auth_selector) input isbuilt with
satisfied=Falseregardless of anything stored for thetask, and the report's top-level
satisfiedis the logical AND ofevery required input across every section. The practical effect: for
as long as a connector declares any required
secretsinput, thetop-level
satisfiedon both read endpoints and on the task-createresponse is always
false, andsecrets_expires_atis alwaysnull-- both are constants of this phase, not a bug in this PR. A later
phase that adds a real secret store gives both fields a real value
without changing their meaning or making either optional.
Testing
pytest tests/web/test_connector_runtime_entrypoints_e2e.py tests/web/api/test_websocket_preview.py -q-- 58 passed.ruff check .-- clean on the files this PR touches.